Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added initial implementation of loader - spinner #1631

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

nitinja
Copy link
Contributor

@nitinja nitinja commented Jul 22, 2021

No description provided.

@nitinja nitinja requested a review from khawkins98 July 22, 2021 10:57
@nitinja
Copy link
Contributor Author

nitinja commented Jul 22, 2021

I am working on eleventy page demo & react demo for this component

@netlify
Copy link

netlify bot commented Jul 22, 2021

✔️ Deploy Preview for ecstatic-noether-150ab4 ready!

🔨 Explore the source changes: 540597e

🔍 Inspect the deploy log: https://app.netlify.com/sites/ecstatic-noether-150ab4/deploys/612ce107e240fd0007e2e9c2

😎 Browse the preview: https://deploy-preview-1631--ecstatic-noether-150ab4.netlify.app

@nitinja
Copy link
Contributor Author

nitinja commented Jul 22, 2021

Eleventy page showing preview correctly in dev mode, but not on deployed preview
image

@sandykadam
Copy link
Collaborator

sandykadam commented Jul 22, 2021

@khawkins98 @nitinja just curious to know if we can build our own EMBL style loader/spinner instead of standard one.

Like we can use the hexagon icon of logo and circle around it animated effect.

https://monophy.com/gifs/embl-embllogo-embllogomonochrome-SU8RkgEO4bcb9bL1U9

https://www.embl.org/news/wp-content/uploads/2014/09/embl_virus_logo_animation_5sec.gif

@nitinja
Copy link
Contributor Author

nitinja commented Jul 22, 2021

Animation

@sandykadam
Copy link
Collaborator

Ah looks nice 👍

Lerna will bump this to alpha.1 on release

<div class="vf-indicator-loader--spinner__circle"></div>

{% if example == true %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

further above you used {% if container == true %}... did you mean to container here as well?

.vf-indicator-loader--spinner__circle {
width: 32px;
height: 32px;
color: $vf-indicator-loader--border-color;
Copy link
Contributor

@khawkins98 khawkins98 Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some suggestions on how to use the design tokens here. Might be clearest/best if I just do that and then we can look at the diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nitinja I still need to update this Sass, but if you could please look at the other PR comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed this with commit 0c370a6

@khawkins98
Copy link
Contributor

Looking good @nitinja — I did have a few minor things that I've noted.

@sandykadam
Copy link
Collaborator

Looking good @nitinja — I did have a few minor things that I've noted.

Are we good to use hexagon style or grey circle loader

@nitinja
Copy link
Contributor Author

nitinja commented Jul 28, 2021

@sandykadam I have committed just the circle spinner as of now, I think we can't commit any design that's not approved by UX people.

This does a couple of things:

1. Sass linting to alphabitise ordering
2. use CSS vars (custom props) — we tend to use this over the Sass vars as this allows more possibility to cascade in new values for styling
@khawkins98
Copy link
Contributor

Adding a note here that based on our recent conversation with @cindyebi and @nitinja this will be part of vf-progress-indicator, and specifically a vf-progress-indicator--spinner variant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants